Skip to content

fix(gsd): prevent auto-mode setModel calls from persisting to settings.json#3486

Open
deseltrus wants to merge 5 commits intogsd-build:mainfrom
deseltrus:fix/auto-setmodel-persist
Open

fix(gsd): prevent auto-mode setModel calls from persisting to settings.json#3486
deseltrus wants to merge 5 commits intogsd-build:mainfrom
deseltrus:fix/auto-setmodel-persist

Conversation

@deseltrus
Copy link
Copy Markdown
Contributor

@deseltrus deseltrus commented Apr 4, 2026

Problem

Auto-mode temporarily switches models for hook dispatch and stop/restore operations. These transient setModel calls persist the model choice to settings.json, silently overwriting the user's default model preference.

Root cause: ExtensionAPI.setModel() in loader.ts drops the options argument, so { persist: false } never reaches the session layer.

Secondary leak: _applyThinkingLevel() unconditionally appends thinking_level_change to session history, which on resume re-persists via setThinkingLevel().

Closes #4339

Fix

Three layers:

  1. loader.ts — forward options through setModel wrapper
  2. auto.ts — pass { persist: false } for all transient model switches
  3. agent-session.ts — gate appendThinkingLevelChange() and setDefaultThinkingLevel() behind persist check

Tests

  • Structural guard: all setModel calls in auto.ts must include persist: false
  • loader.ts: options forwarding unit test
  • agent-session.ts: thinking-level history entry gated by persist option
  • appendThinkingLevelChange index vs persist-guard index ordering test

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 4, 2026

🔴 PR Risk Report — CRITICAL

Files changed 6
Systems affected 4
Overall risk 🔴 CRITICAL

Affected Systems

Risk System
🔴 critical Agent Core
🔴 critical State Machine
🔴 critical Auto Engine
🟠 high Extensions
File Breakdown
Risk File Systems
🔴 packages/pi-coding-agent/src/core/agent-session.ts Agent Core, State Machine
🟠 packages/pi-coding-agent/src/core/extensions/loader.ts Extensions
🔴 src/resources/extensions/gsd/auto.ts Auto Engine
packages/pi-coding-agent/src/core/agent-session-model-switch.test.ts (unclassified)
packages/pi-coding-agent/src/core/extensions/loader-model-options.test.ts (unclassified)
src/resources/extensions/gsd/tests/model-isolation.test.ts (unclassified)

⚠️ Critical risk — please verify: state persistence, auth token lifecycle, agent loop race conditions, RPC protocol compatibility.

@jeremymcs
Copy link
Copy Markdown
Collaborator

Adversarial Review — persist: false thinking-level leak

Verdict: needs-attention

Finding (high severity)

The persist: false path added for transient auto-mode model switches is not fully isolated for thinking level.

_applyThinkingLevel (agent-session.ts:1748-1751) still appends thinking_level_change entries to durable session history even when options.persist === false. On session resume, replay logic calls setThinkingLevel(...) which writes back to settings by default — meaning a transient auto-mode model/thinking switch can survive in session state and silently overwrite the user's saved default in settings.json.

This defeats the intended isolation fix.

Recommendation

  1. Don't record thinking-level changes as durable thinking_level_change entries when persist: false, or mark them as transient and skip them during session replay.
  2. Ensure resume applies replayed thinking with a non-persisting path (e.g., internal setter with { persist: false }) so replay cannot mutate settings.json.

Suggested test

Add an integration test that:

  1. Triggers setModel(..., { persist: false })
  2. Reloads/resumes the session
  3. Asserts settings.json default thinking level is unchanged

Review generated via Codex adversarial review

@jeremymcs
Copy link
Copy Markdown
Collaborator

This PR has merge conflicts with the base branch. Please rebase or merge main to resolve before review can proceed.

🤖 Automated PR audit — 2026-04-04

@deseltrus deseltrus force-pushed the fix/auto-setmodel-persist branch from f00cff5 to 46cf305 Compare April 5, 2026 05:03
@deseltrus
Copy link
Copy Markdown
Contributor Author

Rebased this branch onto current main by rebuilding it from a fresh origin/main worktree and replaying only the persistence fix commits.

What changed in the refresh:

  • resolved the branch/base conflicts without carrying unrelated diffs
  • kept the original three fix hunks intact
  • added two small regression guards for the actual failure seams:
    • ExtensionAPI.setModel(..., options) must forward persist: false
    • transient model switches must not persist clamped thinking-level defaults

Local verification on the refreshed branch:

  • targeted tests: node --import ./src/resources/extensions/gsd/tests/resolve-ts.mjs --experimental-strip-types --test packages/pi-coding-agent/src/core/agent-session-model-switch.test.ts packages/pi-coding-agent/src/core/extensions/loader-model-options.test.ts src/resources/extensions/gsd/tests/model-isolation.test.ts
  • full build: npx -y node@22.13.0 $(npm root -g | sed 's|/node_modules$||')/node_modules/npm/bin/npm-cli.js run build

CI should now be running on the cleaned branch head.

@deseltrus deseltrus force-pushed the fix/auto-setmodel-persist branch from 46cf305 to cfaee54 Compare April 5, 2026 16:18
@deseltrus
Copy link
Copy Markdown
Contributor Author

Addressed the adversarial review finding — good catch.

Root cause: appendThinkingLevelChange() (line 1752) ran unconditionally, even when persist: false. This created a durable thinking_level_change entry in session history. On session resume, replay calls the public setThinkingLevel() which has no persist: false parameter — silently re-persisting the transient change to settings.json.

Fix: Moved appendThinkingLevelChange() inside the if (options?.persist !== false) guard, alongside setDefaultThinkingLevel(). Now transient model switches leave no trace in session history at all.

Added regression test: Structural assertion verifying appendThinkingLevelChange appears AFTER the persist guard check — catches future regressions where someone might move it back outside.

All 4 model-switch tests + 13 model-isolation tests pass. Branch rebased on latest main.

@trek-e trek-e added the bug Something isn't working label Apr 13, 2026
Copy link
Copy Markdown
Collaborator

@trek-e trek-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No linked issue — please open a GitHub issue for this bug and add 'Closes #NNN' to the PR body.

The fix is thorough and addresses all three layers correctly:

  1. loader.ts — the root cause fix. The ExtensionAPI setModel wrapper was silently dropping the options argument, rendering every downstream persist guard ineffective. The one-line fix is correct.

  2. auto.ts — the two setModel calls (stopAuto model restore and dispatchHookUnit hook model) now pass { persist: false }. Correct.

  3. agent-session.ts — _applyThinkingLevel now gates both appendThinkingLevelChange and setDefaultThinkingLevel behind the persist check. The structural test (appendIdx > persistGuardIdx) verifies the ordering is correct and will catch any future regression that moves the append call outside the guard.

The package-lock.json version bump from 2.56.0 to 2.63.0 appears unrelated to the fix. If this was an accidental rebase artifact, it should be reverted to avoid noise in the diff. If it is intentional, it should be called out in the PR description.

Everything else — the structural guard test in model-isolation.test.ts, the loader test, the thinking-level session history test — is exactly the right coverage for this kind of silent persistence bug.

deseltrus and others added 5 commits April 16, 2026 14:01
The ExtensionAPI.setModel wrapper in loader.ts accepts only the model
parameter, silently dropping the options argument. This means every
pi.setModel(model, { persist: false }) call from any extension becomes
runtime.setModel(model) — the persist guard is lost and transient model
switches always overwrite the user's saved default in settings.json.
_applyModelChange() calls setThinkingLevel() unconditionally, which
always writes the effective thinking level to settings.json. When
auto-mode dispatches use setModel({ persist: false }), the model
default is correctly protected, but the thinking level default is not.

If a transient model switch triggers thinking level clamping (e.g.
xhigh → high), the clamped level is persisted and every subsequent
session starts with the wrong thinking level.

Extract _applyThinkingLevel(level, options?) as a private method that
respects the persist option from _applyModelChange. The public
setThinkingLevel(level) API is unchanged — it delegates to
_applyThinkingLevel without options, preserving the existing persist
behavior for user-initiated changes.
…s.json

Two setModel() calls in auto.ts omit { persist: false }, causing
transient model switches during auto-mode to silently overwrite the
user's saved default model in settings.json. Every subsequent session
then starts with the wrong model.

Adds a structural regression test that fails if any setModel call in
auto.ts is missing persist: false.
Add targeted structural regressions for the two persistence leaks behind the auto-mode settings overwrite bug. The new tests pin the ExtensionAPI setModel wrapper to forward options and assert that transient model switches do not persist thinking-level defaults when persist:false is used.
Address review feedback: appendThinkingLevelChange() was recording
durable entries even for transient (persist:false) model switches.
On session resume, replay would call the public setThinkingLevel()
which always persists — silently overwriting the user's saved default.

Move appendThinkingLevelChange inside the persist guard so transient
thinking-level changes leave no trace in session history.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@deseltrus deseltrus force-pushed the fix/auto-setmodel-persist branch from cfaee54 to ebe61b7 Compare April 16, 2026 12:03
Copy link
Copy Markdown
Collaborator

@trek-e trek-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior feedback fully addressed. LGTM.

  • Linked issue added (Closes #4339). Done.
  • package-lock.json version bump artifact is no longer present in the diff. Done.
  • All three fix layers are correct (loader.ts options forwarding, auto.ts persist:false calls, agent-session.ts thinking-level guard).
  • Regression tests cover all the important seams.
  • Full CI is green (build, lint, integration-tests, windows-portability, all rtk-portability variants — all passing as of 2026-04-16).

Copy link
Copy Markdown
Collaborator

@trek-e trek-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES | Severity: MAJOR

The three-layer fix correctly addresses the stated problem. However there is one real bug and one structural test weakness that need resolution before merge.


MAJOR — _emitSessionStateChanged("set_thinking_level") fires even on no-op persist:false paths

In the new _applyThinkingLevel, _emitSessionStateChanged("set_thinking_level") sits outside the if (options?.persist !== false) guard but still inside if (isChanging). When called with persist: false, the event fires and any subscribers (UI, telemetry) will observe a thinking-level-changed signal even though no durable change was made. Whether this is a regression depends on pre-existing behavior — but the intent of persist: false is "transient, leave no trace", and an emitted session-state event is a trace. This should either be gated behind the same persist check or explicitly documented as intentional.


MAJOR — structural test for auto.ts is bypassable with multiline calls

tests/model-isolation.test.ts uses:

const setModelCalls = autoSrc.match(/\.setModel\([^)]*\)/g) ?? [];

[^)]* does not span newlines, so any setModel call formatted across multiple lines:

await pi.setModel(
  match,
  { persist: false },
);

produces \.setModel( with no closing ) in the match, yielding an empty array or a truncated match that fails to include persist: false. The test would then silently pass (the call isn't matched) or spuriously fail. This is the enforcement guarantee for the entire PR — if it can be bypassed by a formatting change the guard is hollow.

Fix: use a multiline-aware regex or read the AST. At minimum, use /\.setModel\([\s\S]*?\)/g with care for nested parens.


MINOR — setThinkingLevel public method is now a thin wrapper with no independent tests

The public API contract (setThinkingLevel always persists) is implicit — it just calls _applyThinkingLevel(level) with no options, so options?.persist !== false is true. This is correct, but it's worth a single explicit test asserting that calling the public method writes to settingsManager. Currently the tests only probe source text structure, not runtime behavior.


What's correct:

  • loader.ts options forwarding is minimal and obviously right.
  • auto.ts call sites — both stopAuto and dispatchHookUnit — are correctly updated.
  • appendThinkingLevelChange being inside the persist guard is the right fix for the session-resume re-persistence loop described in the PR.
  • The comment on _applyModelChange's existing persist !== false guard for setDefaultModelAndProvider confirms this PR is consistent with the prior guard pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: auto-mode model switches persist to settings.json

3 participants